Skip to content

gh-132113: Make EnvironmentVarGuard thread safe - #132128

Closed
srinivasreddy wants to merge 6 commits into
python:mainfrom
srinivasreddy:thread_safe_env_var_guard
Closed

srinivasreddy wants to merge 6 commits into
python:mainfrom
srinivasreddy:thread_safe_env_var_guard

Conversation

@srinivasreddy

@srinivasreddy srinivasreddy commented Apr 5, 2025 •

Copy link
Copy Markdown
Contributor

@brianschubert

brianschubert commented Apr 5, 2025 •

Copy link
Copy Markdown
Contributor

Looks related to #132113 - can you add the issue number to the title?

edit: re "Please do not review yet", you can mark this PR as a draft by clicking the "Convert to draft" button under "Reviewers" in the right-hand column

@srinivasreddy srinivasreddy changed the title Make EnvironmentVarGuard thread safe gh-132113: Make EnvironmentVarGuard thread safe Apr 5, 2025
@srinivasreddy
srinivasreddy marked this pull request as draft April 5, 2025 14:39
@srinivasreddy
srinivasreddy force-pushed the thread_safe_env_var_guard branch from fbe6202 to a8a295f Compare April 5, 2025 18:43
@srinivasreddy
srinivasreddy marked this pull request as ready for review April 5, 2025 18:46
@Wulian233

Copy link
Copy Markdown
Contributor

Could you add a news entry? thanks!

@python-cla-bot

python-cla-bot Bot commented Apr 6, 2025

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.

CLA signed

@picnixz

picnixz commented Apr 7, 2025 •

Copy link
Copy Markdown
Member

Can you add a test please? and/or confirm that the reproducer given on the issue is fixed?

Comment thread Lib/test/test_support.py
for v in linked:
self.assertIsInstance(v, int)

def test_threadsafe_environmentvarguard(self):

@picnixz picnixz Apr 7, 2025 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test should be executed more than once. I think, so I would suggest doing something like that (when I executed the script in my terminal, I needed more than one script execution to see the issue)

def test_threadsafe_environmentvarguard(self):
    for _ in range(100):
        self._test_threadsafe_environmentvarguard()

def _test_threadsafe_environmentvarguard(self):
    ...

@picnixz

picnixz commented Apr 18, 2025

Copy link
Copy Markdown
Member

Following Serhiy's explanation and considering I actually didn't think enough about this, I closed the issue.

@picnixz picnixz closed this Apr 18, 2025
@srinivasreddy
srinivasreddy deleted the thread_safe_env_var_guard branch April 18, 2025 01:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants